Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add String method to strip <br> tags from markdown #388

Closed
wants to merge 1 commit into from
Closed

Add String method to strip <br> tags from markdown #388

wants to merge 1 commit into from

Conversation

thenickcox
Copy link

Currently, the docs are being compiled with the markdown parsing adding a &lt;br&gt; tag for every new line. This will have one of two effects. Either a) The docs are formatted with line breaks added for readability in the Markdown and the compiled HTML becomes cluttered with extraneous tags, and the layout styling is messed up, or b) The docs can be written without line breaks to preserve the intended page layout, but this creates long paragraphs, sacrificing readability.

That's not a choice we should have to make!

Excerpt of docs before this PR:

<h2><a name="application-events" class="anchor" href="#application-events"><span class="header-link"></span></a>Application Events</h2>
<p>The <code>Application</code> object raises a few events during its lifecycle, using the
<br>
<a href="./marionette.functions.html#marionettetriggermethod">Marionette.triggerMethod</a> function. These events
<br>
can be used to do additional processing of your application. For example, you
<br>
may want to pre-process some data just before initialization happens. Or you may
<br>
want to wait until your entire application is initialized to start
<br>
<code>Backbone.history</code>.</p>

After:

<h2>
  <a name="application-events" class="anchor" href="#application-events">
    <span class="header-link"></span>
  </a>
  Application Events
</h2>
<p>The <code>Application</code> object raises a few events during its lifecycle, using the
<a href="./marionette.functions.html">Marionette.triggerMethod</a>
function. These events can be used to do additional processing of your application.
For example, you may want to pre-process some data just before initialization happens.
Or you may want to wait until your entire application is initialized to start
<code>Backbone.history</code>.</p>

Closes #384.

@thenickcox
Copy link
Author

This broke the build on the "compileAnnotatedSrc:marionette" (compileAnnotatedSrc) task, but that succeeds for me locally (as part of npm run compile-all). Any ideas?

Update

The failure is The command "./travis-runner.sh" exited with 128.. That apparently means invalid argument to exit. Not sure why my commit broke this.

Update 2

Just to see, I took out the calls to stripBrTags() in the compile-docs.js task (meaning the only change was adding the string method stripBrTags() that never gets called and thus should have no effect), and the build failed with the same exit code. I then removed the String.prototype.stripBrTags() method altogether, and the build still failed.

The builds seem to have been failing for a while, but the commit on master merging the PR that broke the build is in master, and the build succeeds. I'm super confused.

@thenickcox thenickcox force-pushed the strip_br_tags branch 2 times, most recently from 693e8ed to 1aec344 Compare April 1, 2015 05:09
@samccone
Copy link
Member

@thenickcox i am so sorry for the delay, mind a rebase off of master?

@thenickcox
Copy link
Author

@samccone No problem about the delay. Although because it's been so long, I just read through this issue and the related ones to remind myself what was happening, and somehow the diff for this commit is just // test, which I'm guessing I did to force Travis to re-run. (I'm also guessing I didn't have privileges to re-run the suite from my branch or something…?) I wonder if I lost the commit that actually stripped the tags. I don't have access to the machine from which I made the commit, so I can't use git reflog or anything. I'll take another look and see what I can do here.

@samccone
Copy link
Member

samccone commented Jan 1, 2016

@thenickcox if you rebase off of master and repush the tests will run :)

@thenickcox
Copy link
Author

@samccone Looking back at the discussion, I seem to recall my fix involving a regex to strip out <br> tags, which I didn't love at the time, and I dislike even more strongly now, because I don't think the marked library adds those by default. I'd love to spend some more time investigating why that's occurring, and see if maybe there's a fix for the configuration of that library or something rather than taking them out after the fact. Unfortunately, I probably won't be able to get to it for a few days. If someone else commits a fix before them, more power to them! If not, I'd love to check this out.

@jdaudier
Copy link
Contributor

@thenickcox Would it ok for me to close this or are you planning to work on this still?

@thenickcox
Copy link
Author

@jdaudier Be my guest!! Thanks!

@thenickcox
Copy link
Author

@jdaudier Oops. Misread. I thought you meant you were going to work on it. Sure, go ahead and close. Hopefully I'll get some time down the road and I'll reopen.

@jdaudier jdaudier closed this May 31, 2016
@paulfalgout paulfalgout mentioned this pull request Jun 3, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Line breaks in the markdown docs are translated as <br /> tags erroneously.
3 participants
@thenickcox @samccone @jdaudier